Correctly resolve tags for function overloads #30253

jeanp413 · 2019-03-07T06:32:00Z

DanielRosenwasser · 2019-03-07T19:33:31Z

There's a common thing people do where they only add JSDoc to the first overload. My feeling is that we should first try to get JSDoc for the applicable overload, and if we don't find anything, we should try to find JSDoc for any other overload. Thoughts @mjbvz @RyanCavanaugh?

ajafff · 2019-03-07T21:04:42Z

I typically don't add jsdoc comments, but when I do, I use it to deprecate a single overload.

DanielRosenwasser · 2019-03-07T21:54:43Z

@ajafff please post future comments in the form of a captioned image meme if possible.

sandersn · 2019-07-03T17:08:23Z

@DanielRosenwasser That's a good point. I'm going to look at Definitely Typed.

So far, it looks like the numbers are really low compared to compared to the total number of overloads. Admittedly, I'm only looking at d.ts and DT is relatively well-documented.

Some super early numbers:
Two-overload sets: 50 of 1,000,000 only document the first overload.
Three-overload sets: 25 of 5000.

I'll run over all of DT (and its dependencies) but right now I think this is a good change.

sandersn

One suggestion, plus a request for more tests.

sandersn · 2019-07-03T17:09:01Z

src/services/symbolDisplay.ts

@@ -501,7 +501,7 @@ namespace ts.SymbolDisplay {

        if (!documentation) {
            documentation = symbol.getDocumentationComment(typeChecker);


I think you should add the same code here.

sandersn · 2019-07-03T17:11:26Z

tests/cases/fourslash/quickInfoJsDocTagsFunctionOverload.ts

+/////**
+//// * @tag Tag text
+//// */
+////declare function /*2*/foo(x: number): void


this test is a good start.

can you add tests for documentation as well?

can you add a test with the sequences [doc/no-tag, doc/tag], [doc/no-tag, doc/no-tag], [no-doc/no-tag, doc/tag], [no-doc/no-tag, doc/no-tag], [no-doc/no-tag, no-doc/tag]? There are other combinations but those are the ones that come to mind.

I couldn't get this to work in my editor when I played with it locally, although the tests pass. Probably I just set it up wrong, but you might want to make sure it at leasts works in your editor.

sandersn · 2019-07-03T20:32:55Z

Actually, looks like I was asking the wrong question. Turns out the number of first-jsdoc-only overloads is quite high, of the overloads with jsdoc:

The yellow line is overloads with any jsdoc; the red line is overloads with jsdoc only on the first signature. Blue is all overloads. X=size of overload set; Y=number of each size of set.

Looks like @DanielRosenwasser is right; the jsdoc for the first signature of the overload should be applied to all others that do not themselves have jsdoc. The same is not true for non-first signatures.

jeanp413 · 2019-07-15T06:37:29Z

@sandersn Pushed a better fix and added more unit tests

…ong-tag

sandersn

Code looks good. The tests are subtly wrong in a couple of places. Can you investigate? If you no longer have time, let me know and I'll take a look.

sandersn · 2020-01-09T20:50:56Z

tests/cases/fourslash/commentsOverloads.ts

            { name: "f3", text: "function f3(a: number): number (+1 overload)" },
            { name: "f4", text: "function f4(a: number): number (+1 overload)", documentation: "this is signature 4 - with number parameter" },
        ],
    },
    {
        marker: "18",
        includes: [
-            { name: "i1_i", text: "var i1_i: i1\nnew (b: number) => any (+1 overload)" },
+            { name: "i1_i", text: "var i1_i: i1\nnew (b: number) => any (+1 overload)", documentation: "new 1" },


this documentation is supposed to be on new (a: string), not new (b: number).

I took a look at this and it's working as intended following:

the jsdoc for the first signature of the overload should be applied to all others that do not themselves have jsdoc

Is this an exception to the above rule?
Should this exception only be applied when useConstructSignatures === true?

TypeScript/src/services/symbolDisplay.ts

Lines 222 to 226 in 376acad

if (useConstructSignatures) {

displayParts.push(keywordPart(SyntaxKind.NewKeyword));

displayParts.push(spacePart());

}

addSignatureDisplayParts(signature, allSignatures, TypeFormatFlags.WriteArrowStyleSignature);

No, I just missed that rule.

sandersn · 2020-01-09T21:01:01Z

tests/cases/fourslash/commentsOverloads.ts

@@ -363,7 +363,7 @@ verify.signatureHelp({ marker: "38", overloadsCount: 2, docComment: "this is sig
 verify.quickInfoAt("38q", "var i3_i: i3\n(a: number) => number (+1 overload)", "this is signature 1");

 verify.signatureHelp({ marker: "39", overloadsCount: 2 });
-verify.quickInfoAt("39q", "var i3_i: i3\n(b: string) => number (+1 overload)");
+verify.quickInfoAt("39q", "var i3_i: i3\n(b: string) => number (+1 overload)", "this is signature 1");


same: this documentation is supposed to be on (a: number) not (b: string).

sandersn · 2020-01-09T21:04:03Z

tests/cases/fourslash/commentsOverloads.ts

@@ -428,7 +428,7 @@ verify.signatureHelp({ marker: "57", overloadsCount: 2, docComment: "c2 1" });
 verify.quickInfoAt("57q", "constructor c2(a: number): c2 (+1 overload)", "c2 1");

 verify.signatureHelp({ marker: "58", overloadsCount: 2 });
-verify.quickInfoAt("58q", "constructor c2(b: string): c2 (+1 overload)");
+verify.quickInfoAt("58q", "constructor c2(b: string): c2 (+1 overload)", "c2 1");


same: (a: number) is the one with jsdoc, not (b: string)

sandersn

Sorry for the confusion, I didn't re-read the whole thread in enough detail.

* Correctly resolve tags for function overloads. Fixes microsoft#30181 * Better fix for microsoft#30181. Added more unit tests * Fix commentsOverloads tests * Fallback to first signature when doc and tags are empty

Correctly resolve tags for function overloads. Fixes microsoft#30181

0a1090d

RyanCavanaugh requested a review from sandersn April 23, 2019 20:48

RyanCavanaugh assigned sandersn Apr 23, 2019

sandersn requested changes Jul 3, 2019

View reviewed changes

jeanp413 added 2 commits July 9, 2019 00:41

Merge branch 'master' into fix-quick-info-wrong-tag

4896dba

Better fix for microsoft#30181. Added more unit tests

04e33bf

ericyhwang mentioned this pull request Sep 4, 2019

TypeScript auto-complete only shows JSDoc for first function overload microsoft/vscode#80292

Closed

jeanp413 added 3 commits December 31, 2019 21:52

Merge remote-tracking branch 'upstream/master' into fix-quick-info-wr…

0a0bfff

…ong-tag

Fix commentsOverloads tests

d97a0bd

Fallback to first signature when doc and tags are empty

376acad

jeanp413 requested a review from sandersn January 1, 2020 21:12

jeanp413 mentioned this pull request Jan 1, 2020

Doc comment missing when generating declarations for const methods from js files #35933

Closed

sandersn requested changes Jan 9, 2020

View reviewed changes

sandersn approved these changes Jan 10, 2020

View reviewed changes

sandersn merged commit 79dcd3d into microsoft:master Jan 10, 2020

jeanp413 deleted the fix-quick-info-wrong-tag branch January 10, 2020 21:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correctly resolve tags for function overloads #30253

Correctly resolve tags for function overloads #30253

jeanp413 commented Mar 7, 2019 •

edited

Loading

DanielRosenwasser commented Mar 7, 2019

ajafff commented Mar 7, 2019

DanielRosenwasser commented Mar 7, 2019 •

edited

Loading

sandersn commented Jul 3, 2019

sandersn left a comment

sandersn Jul 3, 2019

sandersn Jul 3, 2019

sandersn commented Jul 3, 2019

jeanp413 commented Jul 15, 2019

sandersn left a comment

sandersn Jan 9, 2020

jeanp413 Jan 10, 2020 •

edited

Loading

sandersn Jan 10, 2020

sandersn Jan 9, 2020

sandersn Jan 9, 2020

sandersn left a comment

		@@ -501,7 +501,7 @@ namespace ts.SymbolDisplay {

		if (!documentation) {
		documentation = symbol.getDocumentationComment(typeChecker);

	if (useConstructSignatures) {
	displayParts.push(keywordPart(SyntaxKind.NewKeyword));
	displayParts.push(spacePart());
	}
	addSignatureDisplayParts(signature, allSignatures, TypeFormatFlags.WriteArrowStyleSignature);

Correctly resolve tags for function overloads #30253

Correctly resolve tags for function overloads #30253

Conversation

jeanp413 commented Mar 7, 2019 • edited Loading

DanielRosenwasser commented Mar 7, 2019

ajafff commented Mar 7, 2019

DanielRosenwasser commented Mar 7, 2019 • edited Loading

sandersn commented Jul 3, 2019

sandersn left a comment

Choose a reason for hiding this comment

sandersn Jul 3, 2019

Choose a reason for hiding this comment

sandersn Jul 3, 2019

Choose a reason for hiding this comment

sandersn commented Jul 3, 2019

jeanp413 commented Jul 15, 2019

sandersn left a comment

Choose a reason for hiding this comment

sandersn Jan 9, 2020

Choose a reason for hiding this comment

jeanp413 Jan 10, 2020 • edited Loading

Choose a reason for hiding this comment

sandersn Jan 10, 2020

Choose a reason for hiding this comment

sandersn Jan 9, 2020

Choose a reason for hiding this comment

sandersn Jan 9, 2020

Choose a reason for hiding this comment

sandersn left a comment

Choose a reason for hiding this comment

jeanp413 commented Mar 7, 2019 •

edited

Loading

DanielRosenwasser commented Mar 7, 2019 •

edited

Loading

jeanp413 Jan 10, 2020 •

edited

Loading